feat: collections page [FC-0062] #1281
Conversation
|
Thanks for the pull request, @navinkarkera! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
navinkarkera
force-pushed
the
navin/FAL-3790-collections-page
branch
from
f381881
to
c95ba13
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1281 +/- ##
==========================================
+ Coverage 92.47% 92.49% +0.02%
==========================================
Files 1025 1031 +6
Lines 18979 19112 +133
Branches 4038 3995 -43
==========================================
+ Hits 17551 17678 +127
- Misses 1363 1369 +6
Partials 65 65 ☔ View full report in Codecov by Sentry. |
navinkarkera
force-pushed
the
navin/FAL-3790-collections-page
branch
3 times, most recently
from
d09b29a
to
630bdf3
Compare
| export const useGetSingleDocument = ({ client, indexName, id }: { | ||
| client?: MeiliSearch; | ||
| indexName?: string; | ||
| id: string | number; | ||
| }) => ( | ||
| useQuery({ | ||
| enabled: client !== undefined && indexName !== undefined, | ||
| queryKey: [ | ||
| 'content_search', | ||
| client?.config.apiKey, | ||
| client?.config.host, | ||
| indexName, | ||
| id, | ||
| ], | ||
| queryFn: () => { | ||
| if (client === undefined || indexName === undefined) { | ||
| throw new Error('Required data unexpectedly undefined. Check "enable" condition of useQuery.'); | ||
| } | ||
| return fetchDocumentById({ client, indexName, id }); | ||
| }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Since the meilisearch contains all data related to a collection, I wanted to fetch individual document from it. But meilisearch getDocument method fails with invalid api-key if we use tenant key. Checked everything related to tenant keys, like allowed actions and search_rules but everything seems in order. It works with main api-key but not with tenant keys even if it has same permissions as the main key.
Current Solution: Used collections api to fetch. Although not used, did not remove this api and hook just in case. Can remove this if we don't want to add unused functions.
There was a problem hiding this comment.
My bet is that the tenant key can only be used in the search endpoint. But, as you said, this is not documented anywhere. The only hint is that they only show examples for tenant keys with search.
There was a problem hiding this comment.
If we're just loading a single "thing", I think it's usually better to load it from the REST API anyways - it's guaranteed to be up to date, and we don't need any of the filtering/keyword/sorting features that Meilisearch provides. What's the exact use case here? Getting the collection metadata + tags in a single call instead of two calls?
There was a problem hiding this comment.
@bradenmacdonald Using Meilisearch here gets us the collection metadata including tags, plus its components in a single (multisearch) hit.
If we're going to start using the REST API in some places and the search index in others, I'd rather this was planned and done as part of a separate ticket. E.g. if we need to make upserting to Meilisearch async, but always want to be able to see the most recent changes made to a library/collection/component, then using the REST API to get after creating/updating makes sense. But while we're upserting synchronously, using the search index alone is fine.
There was a problem hiding this comment.
@bradenmacdonald @pomegranited Like Jill mentioned, currently we are getting everything for this page via a single multi-search query call to meilisearch.
This useGetSingleDocument is not used.
There was a problem hiding this comment.
Just a note that I'm changing to use the REST API to get the collection metadata here: #1320
Everything was fine when we had only two SearchContext: one for the library page with everything and one modified in the collection page (changing the collection query to get only the current collection record). I always could get the updated collection info via SearchContext.
But the collection sidebar used the data from the meilisearch passed as props (the CollectionHit).
When I added the CollectionSidebar on the library page, I passed the CollectionHit from the card onClick.
This caused some issues when I updated the Collection because the data from the list was updated (invalidating the data on the SearchContext), but I didn't have a clean way to update the CollectionHit that was passed as props, resulting in the sidebar being out of sync.
There was a problem hiding this comment.
That makes sense to me.
navinkarkera
force-pushed
the
navin/FAL-3790-collections-page
branch
from
64e8d62
to
7d35166
Compare
| <Container size="xl" className="px-4 mt-4 mb-5 library-authoring-page"> | ||
| <SearchContextProvider | ||
| extraFilter={[`context_key = "${libraryId}"`, `collections.key = "${collectionId}"`]} | ||
| fetchCollections={false} |
There was a problem hiding this comment.
I don't quite understand why we need this flag if we have to add useCollection to fetch the collection anyway?
I would have thought that those extraFilters you're sending to SearchContextProvider would ensure that the first result in the collection query (multisearch number 3) is the Collection we want to show here.
Ah sorry.. the second filter is collections.key=collectionId, and for this to work we'd need block_id = collectionId.. I guess it could be an "OR" statement, but I'm not sure that complexity is worth it here.
There was a problem hiding this comment.
@pomegranited useCollection gets collection data from API while this flag controls fetching of all collections based on search. We only want to fetch blocks under this collection which is why we have update the extraFilter. As I mentioned in the comment above, there seems to be some issue with fetching single document from meilisearch.
But I do like the idea of using block_id field which is currently set to collection.key but adding content_key i.e. library.key into the filter we can get an individual document. If you agree, I can update it and remove the need to calling collections api to fetch single collection data.
There was a problem hiding this comment.
Oh yes please -- it would be better if we could use meilisearch's data instead of hitting the backend, since we're doing that everywhere else.
There was a problem hiding this comment.
@pomegranited Done. Had to include block_id to filterable attributes in edx-platform and update search manager to override queries.
There was a problem hiding this comment.
Noted, and thanks for making that change @navinkarkera . We just need to remember to run reindex_search on the tagging sandbox when we deploy this change for testing.
navinkarkera
force-pushed
the
navin/FAL-3790-collections-page
branch
4 times, most recently
from
aab263b
to
356a26e
Compare
There was a problem hiding this comment.
👍 Spectacular work here @navinkarkera ! Thank you for addressing so many issues in one PR.
These issues will be handled in separate tickets:
- Collections Page: Collections Sidebar #1089 -- FYI @rpenido
- (issue TBD) Pasting components into a collection
- I tested this with a new collection and adding new components, and sorting/filtering/searching the collection's components.
- I read through the code
- I checked for accessibility issues by using my keyboard to navigate and checking color contrasts.
- Includes documentation
- User-facing strings are extracted for translation
| const goBack = React.useCallback((prevPath?: string) => { | ||
| if (prevPath) { | ||
| // Redirects back to the previous route like collection page or library page | ||
| navigate(prevPath); |
There was a problem hiding this comment.
| navigate(prevPath); | |
| navigate(-1); |
If you want to go back, it's not necessary to pass prevPath through all these callbacks like you've done here. You can just call navigate(-1). I actually had this in an earlier PR, but @rpenido suggested changing it.
For now I don't really care either way because what we really need to do is refactor this to display the editor as a modal, instead of navigating to a new URL for the editor and then going back. Obviously that will have to come in a dedicated PR though.
There was a problem hiding this comment.
See my other comment.
| onClose={onClose} | ||
| returnFunction={returnFunction} | ||
| onClose={onClose ? () => onClose(location.state?.from) : null} | ||
| returnFunction={returnFunction ? () => returnFunction(location.state?.from) : null} |
There was a problem hiding this comment.
See my other comment - I don't think it's necessary to pass location.state?.from here.
Though it may actually solve @rpenido's concern about not going back to a different website.
There was a problem hiding this comment.
@bradenmacdonald Yes, my first solution was to use navigate(-1) but it goes back to other website if we directly use the URL and has no way of checking history before moving back, so I used this option of passing state where we can be sure that it goes back to libraries even if the user directly jumps to the url.
There was a problem hiding this comment.
@pomegranited @bradenmacdonald Thanks for your insights, I have updated the PR to make additional api call for adding components to a collection.
| onClose={onClose} | ||
| returnFunction={returnFunction} | ||
| onClose={onClose ? () => onClose(location.state?.from) : null} | ||
| returnFunction={returnFunction ? () => returnFunction(location.state?.from) : null} |
There was a problem hiding this comment.
@bradenmacdonald Yes, my first solution was to use navigate(-1) but it goes back to other website if we directly use the URL and has no way of checking history before moving back, so I used this option of passing state where we can be sure that it goes back to libraries even if the user directly jumps to the url.
| const goBack = React.useCallback((prevPath?: string) => { | ||
| if (prevPath) { | ||
| // Redirects back to the previous route like collection page or library page | ||
| navigate(prevPath); |
There was a problem hiding this comment.
See my other comment.
| export const useGetSingleDocument = ({ client, indexName, id }: { | ||
| client?: MeiliSearch; | ||
| indexName?: string; | ||
| id: string | number; | ||
| }) => ( | ||
| useQuery({ | ||
| enabled: client !== undefined && indexName !== undefined, | ||
| queryKey: [ | ||
| 'content_search', | ||
| client?.config.apiKey, | ||
| client?.config.host, | ||
| indexName, | ||
| id, | ||
| ], | ||
| queryFn: () => { | ||
| if (client === undefined || indexName === undefined) { | ||
| throw new Error('Required data unexpectedly undefined. Check "enable" condition of useQuery.'); | ||
| } | ||
| return fetchDocumentById({ client, indexName, id }); | ||
| }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
@bradenmacdonald @pomegranited Like Jill mentioned, currently we are getting everything for this page via a single multi-search query call to meilisearch.
This useGetSingleDocument is not used.
navinkarkera
force-pushed
the
navin/FAL-3790-collections-page
branch
from
356a26e
to
d388335
Compare
Description
Adds collections page as described in #1101
Also addresses following issues.
Depends on openedx/edx-platform#35493
Supporting information
Private-ref: FAL-3790Testing instructions
Concerns